Big query system cleanups#152636
Conversation
`rustc_query_system` has been reduced so much that it's no longer needed. This avoids a lot of indirection and abstraction.
By removing the generic `D` parameter from `DepGraph`, `DepGraphData`, `CurrentDepGraph`, `SerializedDepGraph`, `SerializedNodeHeader`, and `EncoderState`.
It's no longer needed.
It's no longer needed now that we can access `TyCtxt` directly.
By moving most of it into `DepKind`, and changing two methods into free functions.
|
|
|
Let's queue up the precautionary perf run while I'm reviewing the changes. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Big query system cleanups
| /// Implements [`QueryContext`] for use by [`rustc_query_system`], since that | ||
| /// crate does not have direct access to [`TyCtxt`]. |
There was a problem hiding this comment.
Suggestion: If we're removing this comment, maybe leave behind a FIXME for getting rid of QueryCtxt entirely.
There was a problem hiding this comment.
(Though removing QueryCtxt turns out to not be particularly hard, so if we do it next then there's not much value in a FIXME.)
There was a problem hiding this comment.
Suggestion: If we're removing this comment, maybe leave behind a FIXME for getting rid of
QueryCtxtentirely.
+1, when reviewing I was also wondering why struct QueryCtxt itself wasn't removed.
|
Very minor commit style nit: When removing traits, I like to write “Remove trait |
| } | ||
|
|
||
| fn with_reduced_queries<T>(self, _: impl FnOnce() -> T) -> T; | ||
| pub trait HasDepContext<'tcx>: Copy { |
There was a problem hiding this comment.
Suggestion: Maybe leave behind a FIXME to look into renaming or removing HasDepContext.
There was a problem hiding this comment.
(Though this turns out to also be a fairly easy follow-up.)
|
Looks good, all very mechanical and straightforward. r=me after considering the suggestions, assuming perf is good. |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0ede0e7): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 483.617s -> 481.529s (-0.43%) |
|
To answer all the above questions: I have a todo list I am working through. I have various follow-up commits already written to remove more stuff. I haven't published those yet because I don't want individual PRs to get too big. I'm not bothering to put "FIXME: remove this" comments on things that I know I will be removing soon. I will look at the small perf regressions today. Hopefully they can be fixed with a small tweak to inlining or something like that. |
|
I investigated the perf results.
Based on all this, and the fact that these are good code simplification changes and there are lots more follow-up changes waiting on these, I will declare this perf loss as negligible and acceptable and not worth further investigation. |
|
@bors p=1 (conflict-prone, and blocking follow-up work) |
This comment has been minimized.
This comment has been minimized.
Big query system cleanups Recent PRs have moved a lot of code from `rustc_query_system` to `rustc_middle` and `rustc_query_impl`, where this code now has access to `TyCtxt`, e.g. #152419, #152516. As a result, a lot of abstraction and indirection that existed to work around this limitation is no longer necessary. This PR removes a lot of it. r? @Zalathar
|
💔 Test for a61003d failed: CI. Failed job:
|
|
A CI runner flaked out for no observable reason. @bors retry |
|
@bors p=5 |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 1396514 (parent) -> fef627b (this PR) Test differencesShow 6 test diffs6 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard fef627b1ebdc7369ddf8a4031a5d25733ac1fb34 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
#151968 does show how to remove it, but it needs a rebase or two. |
|
Finished benchmarking commit (fef627b): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.7%, secondary -7.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.5%, secondary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 482.657s -> 481.396s (-0.26%) |
Recent PRs have moved a lot of code from
rustc_query_systemtorustc_middleandrustc_query_impl, where this code now has access toTyCtxt, e.g. #152419, #152516. As a result, a lot of abstraction and indirection that existed to work around this limitation is no longer necessary. This PR removes a lot of it.r? @Zalathar